HADOOP-17028. ViewFS should initialize mounted target filesystems lazily#2260
HADOOP-17028. ViewFS should initialize mounted target filesystems lazily#2260abhishekdas99 wants to merge 2 commits intoapache:trunkfrom
Conversation
ebe4957 to
0d01d8d
Compare
|
💔 -1 overall
This message was automatically generated. |
0d01d8d to
fc33848
Compare
|
@umamaheswararao Can you please review this PR when you get a chance |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
left a comment
There was a problem hiding this comment.
I like the lazy eval style
- thread safety: what is the story here?
- what's the story w.r.t IOExceptions being raised in the init function?
in org.apache.hadoop.fs.impl.FunctionsRaisingIOE we have an interface for functions which explicilty raise them. Using that or something like it makes sense here
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
make it a PathIOException with the given path.
There was a problem hiding this comment.
move to java import group
fc33848 to
e6fefc0
Compare
|
💔 -1 overall
This message was automatically generated. |
Thanks @steveloughran for the review. |
e6fefc0 to
c772403
Compare
|
💔 -1 overall
This message was automatically generated. |
steveloughran
left a comment
There was a problem hiding this comment.
Someone else needs to review the core functionality, but I like the changes. Still need to get that sync block right though. FWIW, long init has caused problems with Hive in the past, hence work in s3a to try and reduce the amount of network IO here. Lazy init too hard work to justify though
There was a problem hiding this comment.
nit: add a . to keep javadoc happy
There was a problem hiding this comment.
added . at the end of the line
There was a problem hiding this comment.
this will dual init as thread #2 will block. You need another check inside the sync block so the second thread won't repeat itself
There was a problem hiding this comment.
Added extra check
There was a problem hiding this comment.
log the full stack trace
There was a problem hiding this comment.
Changed the code.
There was a problem hiding this comment.
FYI. I've got a WrappedIOException, but in #2069 making it a public org.apache.hadoop.fs.functional.RuntimeIOException whose cause is only ever IOE. Not something to be picked up here (yet), but worth knowing. I'm trying to make the functional & lambda/expression stuff more useable,
There was a problem hiding this comment.
Changed the code.
|
Thanks @steveloughran for review! I am wondering this call will make all fs to be initialized. That means we are not getting this lazy initialization benefit fully. |
c772403 to
9319754
Compare
|
@umamaheswararao @steveloughran bumping up the conversation on this thread. Recently we have faced lot of issues (OOM, fs initialization time going up) because of not having lazy initialization. |
There was a problem hiding this comment.
It looks like @steveloughran deprecated this class later after his comment. Don't know what prompted the refactoring, but
import org.apache.hadoop.fs.impl.FunctionsRaisingIOE.FunctionRaisingIOE
should be now
org.apache.hadoop.util.functional.FunctionRaisingIOE
There was a problem hiding this comment.
replaced org.apache.hadoop.fs.impl.FunctionsRaisingIOE.FunctionRaisingIOE with org.apache.hadoop.util.functional.FunctionRaisingIOE
There was a problem hiding this comment.
You can call closeAll() here, which clears the cache completely. And the you can compare with 0 in the assert.
73a5661 to
c66c2bf
Compare
|
💔 -1 overall
This message was automatically generated. |
142d2ff to
7708fdd
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
7708fdd to
d8aac16
Compare
|
💔 -1 overall
This message was automatically generated. |
d8aac16 to
2f8e9a6
Compare
|
💔 -1 overall
This message was automatically generated. |
2f8e9a6 to
8fa62c8
Compare
|
💔 -1 overall
This message was automatically generated. |
8fa62c8 to
a658312
Compare
|
💔 -1 overall
This message was automatically generated. |
|
Looks good generally. I checked the tests are catching previous errors.
|
a658312 to
541e432
Compare
@shvachko I have removed the checkstyle warning. Also added the individual imports and removed the empty line |
|
🎊 +1 overall
This message was automatically generated. |
|
+1 on the latest patch. |
|
💔 -1 overall
This message was automatically generated. |
…ily. Contributed by Abhishek Das (#2260)
|
💔 -1 overall
This message was automatically generated. |
…ily. Contributed by Abhishek Das (apache#2260) (cherry picked from commit 1dd03cc)
…ily. Contributed by Abhishek Das (apache#2260)
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
https://issues.apache.org/jira/browse/HADOOP-17028